-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove power of two check #78
Conversation
64803e6
to
8f79270
Compare
@Sjord is this how you'd like it? |
I see MIRI isn't able to be ran on the compile tests, we should probably add an exclusion for that. Also tarpaulin seems broken for some reason, maybe we need to update that pipeline stage |
The cargo tarpaulin action seems to be quite outdated anyways:
So I want to switch to another system for coverage, but I'll give that its own PR. |
…ringbuffer into remove-power-of-two-requirement
Also enabled non-power-of-two alloc ringbuffers. The ability to have non-power-of-two alloc ringbuffers has to be enabled at compile time as benchmarks show that it can be 2-3 times slower than using ands. By default, alloc ringbuffers still use have the power of two requirement As well as that, made the unchecked capacity version of the alloc ringbuffer unsafe. This is a breaking change. However, using it with a non-power-of-two capacity was literally undefined behavior, so I think we want this to be marked unsafe. |
Yes, this makes sense. My opinion shouldn't weigh too heavily though. I used this crate for Advent of Code day 6. The assignment was to find 4 consecutive different characters, and I used this ringbuffer for that. Then, part 2 of the assignment was to find 14 consecutive different characters, and then I ran into problems that only powers of two were supported. I think it makes sense to support any size, as long as users are aware of the performance consequences. But my use-case was artificial and one-off, so I don't want to steer the direction of this crate too much.
There are also crates that speed up the modulo by doing some calculations in advance, which can be useful to speed up alloced ringbuffers of arbitrary size: |
e195148
to
8fcbb12
Compare
It's also possible to modify |
indeed outside of scope. Would save two words even though |
…ringbuffer into remove-power-of-two-requirement
Replaces #76 (for at least the power of two check), addressing the last commments on #61 by @Sjord. They are completely right that the power of two requirements is actually not really necessary for the const generic ringbuffer as its compiled to the same bitshift anyway
Closes #61